feat(ui): register and display model_type from catalog and model registry#2545
feat(ui): register and display model_type from catalog and model registry#2545Taj010 wants to merge 5 commits intokubeflow:mainfrom
Conversation
Signed-off-by: Taj010 <rsheen003@gmail.com>
…o it Signed-off-by: Taj010 <rsheen003@gmail.com>
Signed-off-by: Taj010 <rsheen003@gmail.com>
| }) => { | ||
| const stored = getModelTypeStoredValueFromCustomProperties(modelCustomProperties); | ||
|
|
||
| const handleChange = (key: string, isPlaceholder: boolean) => { |
There was a problem hiding this comment.
The handleChange callback checks both isPlaceholder and key === MODEL_TYPE_PLACEHOLDER_KEY. The isPlaceholder check alone should be sufficient — checking the key as well is redundant but harmless.
| string_value: next, | ||
| }; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Please add unit tests for this utility.
There was a problem hiding this comment.
Added registerModelTypeUtils.spec.ts
| isSubmitDisabledForCommonFields(formData, namespaceHasAccess, isNamespaceAccessLoading) || | ||
| !isNameValid(formData.modelName) || | ||
| isModelNameExisting(formData.modelName, registeredModels); | ||
| options?: { requireModelType?: boolean }, |
There was a problem hiding this comment.
We've changed the default behavior here which is fine, the only thing I would maybe comment here that if in the future a caller will forget to opt-out it might be surprising . Alternatively we can default to false with explicit opt-in( just safer) not a must, you're choice
There was a problem hiding this comment.
Adjusted to have requireModelType now defaults to false and adjusted the utils.spec.ts too
| * Returns model registry customProperties entries to prefill `model_type` when registering | ||
| * from the catalog. Only generative/predictive are copied; unknown or missing values yield {}. | ||
| */ | ||
| export const getCatalogModelTypePropertyForRegistration = ( |
There was a problem hiding this comment.
In here we can reuse reuse getModelTypeStoredValueFromCustomProperties from registerModelTypeUtils.ts , or even better - buildCustomPropertiesWithModelType
There was a problem hiding this comment.
Reimplemented via getModelTypeStoredValueFromCustomProperties and buildCustomPropertiesWithModelType
Signed-off-by: Taj010 <rsheen003@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Taj010 <rsheen003@gmail.com>
mturley
left a comment
There was a problem hiding this comment.
Nice work! One thing to check: model_type is now displayed in a dedicated "Model type" field on the detail pages, but the getProperties() function in screens/utils.ts doesn't exclude model_type from the editable properties table. This means it will show up twice — once as the new read-only display field, and again as an editable row in the properties section. Should it be filtered out of the properties table (similar to how _lastModified and _registeredFrom are excluded)?
Also I actually think we may want a tech debt ticket to clean those two up, at least _registeredFrom isn't used anymore now that we have modelSource* properties, but that's out of scope.
| > | ||
| <ModelTimestamp timeSinceEpoch={rm.createTimeSinceEpoch} /> | ||
| </DashboardDescriptionListGroup> | ||
| <DashboardDescriptionListGroup title="Model type"> | ||
| <Content component="p" data-testid="registered-model-model-type"> | ||
| {formatModelTypeDisplay(modelTypeRaw)} | ||
| </Content> |
There was a problem hiding this comment.
model_type will also appear in the editable properties table rendered by ModelPropertiesExpandableSection below, since getProperties() in screens/utils.ts doesn't filter it out. This means the user will see it twice — here as read-only, and again as an editable property row. Consider adding model_type to the exclusion list in getProperties().
Description
This PR adds a shared Model type field (Generative / Predictive) on register-from-catalog and register-from-registry flows.
Model overview and version details was also adjusted to show model type property.
How Has This Been Tested?
Manually Testing
Automated Testing
modelCatalogUtils.spec.tsandutils.spec.ts fileand adjusted tests inregisterAndStoreFields.cy.tsMerge criteria:
DCOcheck)ok-to-testhas been added to the PR.If you have UI changes